Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Converting GlobalFilter to Jotai #2955

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

justinorringer
Copy link
Contributor

This will touch a lot of the code base, so I have some questions.

@@ -1,25 +0,0 @@
import * as actionTemplates from './actions';
import chromeReducer, { chromeInitialState } from '.';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need any of this middleware logic?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is the on event which leverages the middlewards. It can be and should done via jotai. https://github.com/Hyperkid123/insights-chrome/blob/master/src/chrome/create-chrome.ts#L100.

It was already done for the APP_NAVIGATION public event. You can follow similar approach.

@@ -37,15 +32,3 @@ const globalFilter = {
[GLOBAL_FILTER_REMOVE]: onGlobalFilterRemove,
[GLOBAL_FILTER_UPDATE]: onTagSelect,
};

export const chromeInitialState: ReduxState = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In create-chrome and other files, there are references to these states. I tried stubbing the state out as unknown and a few variations to make this pr smaller. Any recommendations?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the types to the jotai atoms and reference it there. Most of the existing jotai atoms are just migrated redux state variables. You can check how its done there.

}),
shallowEqual
);
const { count, total, tags, sid, workloads, isLoaded } = useGlobalFilterState();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a hook for this, may be unnecessary?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When consolidating state variables, just be careful you are properly memorizing the state output. To prevent extra re-renders.

I don't see an issue with using the jotai atoms directly.

src/components/GlobalFilter/GlobalFilter.tsx Show resolved Hide resolved
src/@types/types.d.ts Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants